Skip to content

Add more support to mir fn bodies #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

tiif
Copy link
Member

@tiif tiif commented Jul 30, 2025

This PR supports:

  • StorageLive and StorageDead
  • Struct and field projection

@tiif tiif changed the title Support more Add more support to mir fn bodies Jul 30, 2025
Comment on lines +144 to +145
#[grammar(struct {$,v0} as $v1)]
Struct(Vec<ValueExpression>, Ty),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In minirust, struct is represented as a tuple:

    /// An n-tuple, used for arrays, structs, tuples (including unit).
    Tuple(List<ValueExpr>, Type),

I'd prefer not grouping arrays, structs and tuples together in one enum variant, because struct is an adt in rustc, and others are not (although it is technically possible to represent all of them as adt?). It'd be quite bad to do typechecking for both adt and non-adt together.

Comment on lines 303 to 328
#[term($:where $,where_clause { $,variants })]
pub struct AdtDeclBoundData {
/// The where-clauses declared on the ADT,
pub where_clause: Wcs,
pub variants: Vec<AdtDeclVariant>,
}

#[term($name { $,fields })]
pub struct AdtDeclVariant {
pub name: VariantId,
pub fields: Vec<AdtDeclField>,
}

#[term($name : $ty)]
pub struct AdtDeclField {
pub name: AdtDeclFieldName,
pub ty: Ty,
}

#[term]
pub enum AdtDeclFieldName {
#[cast]
Id(FieldId),
#[cast]
Index(usize),
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added the variant information to AdtDeclBoundData because I find it a lot nicer to get struct field type information directly from decl in bffdf5a#diff-be3230c387ee7e1ccef3d2eea52917c384f3bf0f656bcdb10e1f28bf3dcfdb5b.

@tiif tiif marked this pull request as ready for review August 6, 2025 09:31
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good -- I left a few suggestions for improvement, but otherwise r=me.

bail!(
"PlaceExpression::Local: unknown local name `{:?}`",
local_id
)
};
place_ty = ty;
}
Field(field_projection) => {
let Local(ref local_id) = *field_projection.root else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can recursively invoke check_place(&field_projection.root), no?

bail!("The local id used in PlaceExpression::Field is invalid.")
};

let Some(adt_id) = ty.get_adt_id() else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now we are checking for ADTs, but eventually we will want to do normalization here -- so probably this check_place function should become a judgment at that time

bail!("The type used in ValueExpression::Struct must be adt")
};

// Check the validity of the struct.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably want to "prove ty is well-formed" somewhere in here

bail!("This type used in ValueExpression::Struct should be a struct")
}

let struct_field_ty: Vec<Ty> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let struct_field_ty: Vec<Ty> =
let struct_field_tys: Vec<Ty> =

let ty = self.check_value(typeck_env, value_expression)?;

// Make sure the type matches the declared adt.
if ty != declared_ty {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should prove a subtype relation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what you could do is use Wcs::all_sub from that other branch we were working on and prove all of them at once; this would be nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants